Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

settings: made monitors selectable in fullscreen modes #17648

Conversation

everclear72216
Copy link
Contributor

An integer value has been added to the graphics settings and is
pushed though the Platform interface to the Sdl2PlatformWindow.
The integer represents the display index used by SDL to identify
a display device.

The platform window hinterface has been extended to provide a
method which can be used to retrieve the available monitors.
The renderer class has been extended to give access to this
information within the settings logic class.

In the display tab of the settings menu the monitor can be
selected via a dropdown iff a non-windowed mode is selected.

"restart necessary"-logic has been updated to cater for the new
configuration option which can only be applied after restart.

The UI has been adaptet for mods/common and mods/cnc

closes #12363

Thank you for your contribution to OpenRA!

Please be aware that we do not have enough project maintainers to match the rate of contributions, so it may take several days before somebody is able to respond to your Pull Request.

You can help speed up the review process by following a few steps:

  • Make sure that you have read and understand the OpenRA Coding Standard (see https://github.com/OpenRA/OpenRA/wiki/Coding-Standard).
  • Write quality commit messages (see https://chris.beams.io/posts/git-commit/).
  • Only commit changes that directly relate to your Pull Request. Use your Git interface to unstage any unrelated changes to project files, line endings, whitespace, or other files.
  • Review the code diff view below to double check that your changes satisfy the above three points.
  • Use the make test and make check commands to check for (and fix!) any issues that are reported by our automated tests.
  • If you are changing shared mod or engine code, make sure that you have tested your changes in all four default mods.
  • Respond to review comments as soon as you reasonably can. Reviewers will usually prioritize Pull Requests that are still fresh in their minds. Make sure to leave a comment when you push new changes, otherwise GitHub does not automatically notify reviewers!
  • Leave a polite comment asking for reviews if a week or more has passed without feedback.

If you need any help you can ask in the #openra IRC channel on freenode (most active during European evenings).

OpenRA.Platforms.Default/Sdl2PlatformWindow.cs Outdated Show resolved Hide resolved
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good already, just a few smallish comments:

OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Platforms.Default/Sdl2PlatformWindow.cs Outdated Show resolved Hide resolved
OpenRA.Platforms.Default/Sdl2PlatformWindow.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Platforms.Default/Sdl2PlatformWindow.cs Outdated Show resolved Hide resolved
@pchote pchote added this to the Next Release milestone Feb 6, 2020
@pchote
Copy link
Member

pchote commented Feb 6, 2020

It looks like you maybe forgot to push the latest fixes?

@everclear72216
Copy link
Contributor Author

Sorry, I was just hesitant because i forgot to test tiberian dawn, which has a different settings.yaml.

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Just a couple more points, then LGTM.

Can you please also squash the fixups into the first commit?

OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
@everclear72216 everclear72216 force-pushed the feature/make-monitos-selectable-in-fullscreen-modes branch from 64509b6 to e086661 Compare February 8, 2020 13:36
@everclear72216
Copy link
Contributor Author

Took care of the comments and squashed the commits. Reworded commit message.

@dragunoff
Copy link
Contributor

Tested on Ubuntu, works as promised 👍

I'm just wondering whether the display dropdown should be visible on single monitor setups as it serves no purpose in that case 🤔

@pchote
Copy link
Member

pchote commented Feb 8, 2020

We should still show it (otherwise it makes the UI code even more of a nightmare), but should disable the dropdown so its not clickable.

OpenRA.Game/Settings.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/SettingsLogic.cs Outdated Show resolved Hide resolved
@everclear72216 everclear72216 force-pushed the feature/make-monitos-selectable-in-fullscreen-modes branch from e086661 to f02068b Compare February 8, 2020 16:26
@pchote pchote dismissed teinarss’s stale review February 8, 2020 18:14

Requests addressed.

@pchote pchote merged commit 98aef70 into OpenRA:bleed Feb 8, 2020
@everclear72216 everclear72216 deleted the feature/make-monitos-selectable-in-fullscreen-modes branch February 8, 2020 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow choosing which monitor the game should use, on multi-monitor systems
5 participants